Enable UAP Remote Execution for all tests #21014

tarekgh · 2017-06-13T22:02:00Z

This change to enable remote execution of all corefx tests in UAP. Because the remote execution code for UAP is using WinRT APIs and to avoid forcing adding UAP configuration to all test projects, we factor the remote execution code into its own test library (called TestHelper) which is cross compiled.

tarekgh · 2017-06-13T22:03:40Z

@weshaggard @ericstj could you please have a look? this is refactoring the remote execution code to be in its own library to avoid adding UAP configuration to all test projects.

tarekgh · 2017-06-13T22:04:02Z

CC @stephentoub @joperezr @safern @danmosemsft

tarekgh · 2017-06-13T22:05:02Z

Note that, I'll not merge the changes here till we get Helix uses RS2 OS images. so this PR is kind of blocked by that.

tarekgh · 2017-06-13T22:22:19Z

@mmitche OSX legs failed because of out of disk space. I am not sure why portable Linux failed. do you know why?

mmitche · 2017-06-13T22:25:39Z

FYI @dotnet/eng-first

@dotnet-bot test Innerloop OSX10.12 Debug x64 Build and Test
@dotnet-bot test Innerloop OSX10.12 Release x64 Build and Test

Not sure why the portable build failed. The line at the end is "script returned -1". https://ci3.dot.net/blue/organizations/jenkins/dotnet_corefx%2Fmaster%2Fportable-linux-Config_Debug%2BOuterLoop_false_prtest/detail/portable-linux-Config_Debug%20OuterLoop_false_prtest/863/pipeline

I wonder whether the build crashed.

mmitche · 2017-06-13T22:25:51Z

@dotnet-bot test Portable Linux x64 Debug Build

stephentoub · 2017-06-14T02:43:49Z

src/TestHelper/dir.props

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="utf-8"?>


Is there a more descriptive name than TestHelper we could use? CoreFx.TestUtilities or something along those lines?

@weshaggard @ericstj what you think of naming this library? I agree with @stephentoub to have better name as TestHelper is more generic. Having CoreFX in the name can give idea it is corefx specific library.

I agree with @stephentoub as well. CoreFx.TestUtilties is fine by me. As we had discussed before you could consider using Private or Internal in the name if you wanted to discourage its use by product assemblies.

Do we expect that we will put other test stuff in here like PlatformDetection and our custom Asserts? We should actually consider doing that now that we have a test helper library here.

As for a name I don't know if we need the name CoreFX included as I don't know that it buys us much, but I do wonder if we shouldn't have it under the Common folder so it doesn't look like a shipping library.

I want to focus on the remote execution so I would to keep the changes in this PR to the remote execution only and we can later move more test code as PlatformDetection there.

I kept the test library folder with other libraries per @ericstj recommendation. so I am not sure if we have to move it to the common folder? please advise if I should do that.

for naming I'll go with CoreFX.Private.TestUtlities. let me know if you disagree with this name.

I want to focus on the remote execution so I would to keep the changes in this PR to the remote execution only and we can later move more test code to PlatformDetection there.

Sounds good.

I kept the test library folder with other libraries per @ericstj recommendation. so I am not sure if we have to move it to the common folder? please advise if I should do that.

Given that it has a ref and other configurations keeping it here is probably easier, lets do that for now and see if any issues arise.

for naming I'll go with CoreFX.Private.TestUtlities. let me know if you disagree with this name.

That is fine, but I think you want CoreFx.Private.TestUtilities

stephentoub · 2017-06-14T02:45:02Z

src/TestHelper/src/System/Diagnostics/RemoteExecutorTestBase.nouap.cs

+namespace System.Diagnostics
+{
+    /// <summary>Base class used for all tests that need to spawn a remote process.</summary>
+    public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase


I assume none of this code has changed, so I'm not re-reviewing it.

Yes I didn't change any code logic. I had to change some internal specifiers to be public specifier instead.

stephentoub

Thanks for working on this.

safern

LGTM, thanks @tarekgh

weshaggard · 2017-06-14T16:35:22Z

src/TestHelper/ref/TestHelper.cs

@@ -0,0 +1,127 @@
+// Licensed to the .NET Foundation under one or more agreements.


This doesn't look like it was generated by GenAPI. Any reason not to have it generated so we can easily regenerate in the future?

You caught it :-) I'll regenerate using GenAPI

weshaggard · 2017-06-14T16:36:51Z

src/TestHelper/dir.props

+  <Import Project="..\dir.props" />
+  <PropertyGroup>
+    <AssemblyVersion>4.0.0.0</AssemblyVersion>
+    <AssemblyKey>MSFT</AssemblyKey>


We shouldn't use the MSFT Key. For this please use the Test key.

I would also probably use 1.0.0.0 as the assembly version, just because it is different from our shipping libraries.

I'll fix those

weshaggard · 2017-06-14T16:40:59Z

src/TestHelper/src/TestHelper.csproj

+
+  <ItemGroup Condition="'$(TargetGroup)' == 'uap'">
+    <Reference Include="Windows" />
+    <Reference Include="WindowsBase" />


Why does WindowsBase need to be referenced?

I think we can remove WindowsBase. will try that.

weshaggard · 2017-06-14T16:49:08Z

src/TestHelper/src/System/Diagnostics/RemoteExecutorTestBase.nouap.cs

@@ -0,0 +1,66 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


I think I would name this file something like ".process" instead of "nouap", to better call out what it is doing.

I was trying to name the files with the targetGroup id's. but I don't have any objection to rename it as you suggested.

weshaggard · 2017-06-14T16:51:32Z

external/test-runtime/XUnit.Runtime.depproj

@@ -114,7 +114,7 @@
          Condition="'$(TargetGroup)'=='uap'" >

    <PropertyGroup>
-      <UAPToolsPackageVersion>1.0.2</UAPToolsPackageVersion>
+      <UAPToolsPackageVersion>1.0.4</UAPToolsPackageVersion>


Looks like you will need to merge with @joperezr's changes.

Yes I'll merge with @joperezr changes after his PR merge

yup, we already talked about this, we will have to create a new version entirely for his changes to work, so we will rebase this PR once mine is merged.

weshaggard · 2017-06-14T16:54:08Z

src/TestHelper/dir.props

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="utf-8"?>


I want to focus on the remote execution so I would to keep the changes in this PR to the remote execution only and we can later move more test code to PlatformDetection there.

Sounds good.

I kept the test library folder with other libraries per @ericstj recommendation. so I am not sure if we have to move it to the common folder? please advise if I should do that.

Given that it has a ref and other configurations keeping it here is probably easier, lets do that for now and see if any issues arise.

for naming I'll go with CoreFX.Private.TestUtlities. let me know if you disagree with this name.

That is fine, but I think you want CoreFx.Private.TestUtilities

weshaggard · 2017-06-14T16:59:17Z

src/TestHelper/src/System/Diagnostics/RemoteExecutorTestBase.netcore.cs

+    /// <summary>Base class used for all tests that need to spawn a remote process.</summary>
+    public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase
+    {
+        protected static readonly string HostRunnerExecutableName = "dotnet";


While this is further clean-up of what was already there it doesn't look like HostRunnerExecutableName and HostRunnerName are even used anywhere so they can just be deleted. In which case HostRunner is all that is needed.

HostRunnerName is used in the S.D.Process tests. but I'll look at the ones not used outside here and will remove them.

weshaggard · 2017-06-14T17:00:34Z

src/TestHelper/src/System/Diagnostics/RemoteExecutorTestBase.uap.cs

+    /// <summary>Base class used for all tests that need to spawn a remote process.</summary>
+    public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase
+    {
+        protected static readonly string HostRunnerExecutableName = "xunit.runner.uap";


Doesn't look like any of these Host fields are used in the uap case.

I kept it because it is used in other test code like S.D.Process tests. as I mentioned in other comment I'll try to get rid of the fields not used in the test projects

joperezr · 2017-06-14T17:33:40Z

@tarekgh I've merged my PR so you should be able to rebase this and merge it in now.

This change to enable remote execution of all corefx tests in UAP. Because the remote execution code for UAP is using WinRT APIs and to avoid forcing adding UAP configuration to all test projects, we factor the remote execution code into its own test library (called TestHelper) which is cross compiled.

tarekgh · 2017-06-14T20:27:12Z

Thanks all for your review and comments.

This change to enable remote execution of all corefx tests in UAP. Because the remote execution code for UAP is using WinRT APIs and to avoid forcing adding UAP configuration to all test projects, we factor the remote execution code into its own test library (called TestHelper) which is cross compiled. Commit migrated from dotnet/corefx@eb2f543

dnfclas added the cla-already-signed label Jun 13, 2017

tarekgh added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-Infrastructure-libraries labels Jun 13, 2017

safern mentioned this pull request Jun 14, 2017

Add System.ComponentModel.EventBasedAsync tests #20931

Merged

stephentoub reviewed Jun 14, 2017

View reviewed changes

stephentoub approved these changes Jun 14, 2017

View reviewed changes

safern approved these changes Jun 14, 2017

View reviewed changes

ericstj approved these changes Jun 14, 2017

View reviewed changes

weshaggard reviewed Jun 14, 2017

View reviewed changes

tarekgh force-pushed the EnableRemoteExecutorForAllTests branch from 4d03638 to 412bdb6 Compare June 14, 2017 19:28

tarekgh removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 14, 2017

tarekgh merged commit eb2f543 into dotnet:master Jun 14, 2017

safern mentioned this pull request Jun 14, 2017

Fixing System.Threading.Overlapped tests in uap #21068

Merged

karelz modified the milestone: 2.1.0 Jun 15, 2017

tarekgh deleted the EnableRemoteExecutorForAllTests branch May 25, 2018 17:24

		@@ -0,0 +1,127 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -0,0 +1,66 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// The .NET Foundation licenses this file to you under the MIT license.

Enable UAP Remote Execution for all tests #21014

Enable UAP Remote Execution for all tests #21014

Conversation

tarekgh commented Jun 13, 2017

tarekgh commented Jun 13, 2017

tarekgh commented Jun 13, 2017

tarekgh commented Jun 13, 2017

tarekgh commented Jun 13, 2017

mmitche commented Jun 13, 2017

mmitche commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Jun 14, 2017

tarekgh commented Jun 14, 2017

tarekgh Jun 14, 2017 •

edited

Loading